Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture std::bad_alloc on deserializeROSmessage. #665

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

MiguelCompany
Copy link
Collaborator

This should fix crashes coming from serialization mismatches that cannot be detected by Fast CDR

Related to ros2/ros2_documentation#3288 (comment)

@fujitatomoya
Copy link
Collaborator

  • service client (cyclonedds)

cannot get the result from server.

root@tomoyafujita:~/ros2_ws/colcon_ws# RMW_IMPLEMENTATION=rmw_cyclonedds_cpp ros2 run demo_nodes_cpp add_two_ints_client
^C[INFO] [1674755622.413766599] [rclcpp]: signal_handler(signum=2)
[ERROR] [1674755622.414263025] [add_two_ints_client]: Interrupted while waiting for response. Exiting.
  • service server (Fast-DDS)

in this sample, cannot observe the core dump or crash. but it is obvious that de-serialization works unexpectedly.

root@tomoyafujita:~/ros2_ws/colcon_ws# cat DEFAULT_FASTRTPS_PROFILES.xml 
<?xml version="1.0" encoding="UTF-8" ?>
<profiles xmlns="http://www.eprosima.com/XMLSchemas/fastRTPS_Profiles">

  <!-- Default publisher profile -->
  <data_writer profile_name="default publisher profile" is_default_profile="true">
    <qos>
      <data_sharing>
        <kind>AUTOMATIC</kind>
      </data_sharing>
    </qos>
    <historyMemoryPolicy>DYNAMIC</historyMemoryPolicy>
  </data_writer>

  <data_reader profile_name="default subscription profile" is_default_profile="true">
    <qos>
      <data_sharing>
        <kind>AUTOMATIC</kind>
      </data_sharing>
    </qos>
    <historyMemoryPolicy>DYNAMIC</historyMemoryPolicy>
  </data_reader>
</profiles>
root@tomoyafujita:~/ros2_ws/colcon_ws# export RMW_FASTRTPS_USE_QOS_FROM_XML=1
root@tomoyafujita:~/ros2_ws/colcon_ws# RMW_IMPLEMENTATION=rmw_fastrtps_cpp ros2 run demo_nodes_cpp add_two_ints_server 
[INFO] [1674755608.560241436] [add_two_ints_server]: Incoming request
a: -2268473885457029753 b: 1
^C[INFO] [1674755623.645151231] [rclcpp]: signal_handler(signum=2)

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, this should avoid unexpected exceptions during de-serialization.

@fujitatomoya
Copy link
Collaborator

@alsora @mauropasse if possible, would you mind trying this patch to see if it can avoid the program crash? (just sending request to the server can get the program crashed, for me that is the security issue...)

@@ -119,6 +119,11 @@ bool TypeSupport::deserializeROSmessage(
"Fast CDR exception deserializing message of type %s.",
getName());
return false;
} catch (...) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, we would prefer to have specific exceptions listed here, rather than just catch everything.

What exceptions can this deserialization throw?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the original report, it looks like we should just catch std::bad_alloc for now. @MiguelCompany if you make that change and rebase this PR onto the latest, I'll be happy to approve and run CI on it.

@christophfroehlich
Copy link

christophfroehlich commented Nov 13, 2023

This fixes #733 if I build this branch from source in the humble container. Is there any chance that this gets merged and backported to humble?

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the hotfix/exception-on-deserialize branch from 5fa730e to c4f869a Compare January 8, 2024 11:36
Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
@MiguelCompany MiguelCompany force-pushed the hotfix/exception-on-deserialize branch from c4f869a to a479bcd Compare January 8, 2024 11:43
@MiguelCompany MiguelCompany changed the title Capture all exceptions on deserializeROSmessage. Capture std::bad_alloc on deserializeROSmessage. Jan 8, 2024
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more minor changes, then we can run CI on this. Thanks!

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Will run CI on this next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 4d0be32 into ros2:rolling Jan 8, 2024
3 checks passed
@MiguelCompany MiguelCompany deleted the hotfix/exception-on-deserialize branch January 10, 2024 07:34
@christophfroehlich
Copy link

@clalancette would you mind backporting this to iron and humble as well? This would fix #733 then.

@clalancette
Copy link
Contributor

@Mergifyio backport humble iron

Copy link

mergify bot commented Jan 16, 2024

backport humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* Capture bad_alloc on deserializeROSmessage.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 4d0be32)
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
* Capture bad_alloc on deserializeROSmessage.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 4d0be32)
fujitatomoya pushed a commit that referenced this pull request Jan 16, 2024
* Capture bad_alloc on deserializeROSmessage.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 4d0be32)

Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
clalancette pushed a commit that referenced this pull request Jan 16, 2024
* Capture bad_alloc on deserializeROSmessage.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 4d0be32)

Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
alsora pushed a commit to irobot-ros/rmw_fastrtps that referenced this pull request Jul 2, 2024
* Capture bad_alloc on deserializeROSmessage.

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>
(cherry picked from commit 4d0be32)

Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
@naorwaiss
Copy link

hi can i get some help
i try to get topic at my ros2 iron from my jetson nano 20.04 that run ros2 foxy
i get the same errror..

@fujitatomoya
Copy link
Collaborator

@naorwaiss foxy is E.O.L https://docs.ros.org/en/rolling/Releases.html#list-of-distributions, can you try ROS Iron instead of 20.04 that run ros2 foxy?

@naorwaiss
Copy link

um i use jetson nano so i can run ubuntu 20.04 - this mean if i understand it .. that i can install only ros2 foxy because ros2 iron is available to ubuntu 22.04..

@fujitatomoya
Copy link
Collaborator

I think that is the requirement for jetson nano platform? can you ask help for jetson community?

@naorwaiss
Copy link

hi the installation of humble isnt work - can help me to make the bridge between ros2 iron and foxy ?

@fujitatomoya
Copy link
Collaborator

can help me to make the bridge between ros2 iron and foxy ?

cross-distribution communication is not officially supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants